fix(agentex): rejected sessions prompt a re-link, unlinked turns stop pretending, and linking answers the pending question - #416
Merged
Conversation
credential_expires_at is an upper bound, not a guarantee. A session JWT stops being honoured the moment its owner signs out or it is revoked, while the stored expiry still reads months away -- so the gateway handed the agent a dead cookie, every user-scoped tool call 401'd where the agent could only report "I can't reach your Notion", and nothing ever prompted a re-link because locally the credential looked fine. Silent, and indefinite. A linked turn now verifies the stored session against agentex-auth before acting as that user, and a refusal falls back to the bot with sgp_user_id=None -- already the exact condition that triggers a re-link offer, so no new wiring. A REJECTION IS NOT A FAILURE, and conflating them is the danger. A misconfigured or unavailable auth service refuses everyone; concluding "your credential is bad" would tell an entire workspace to re-link, which fixes nothing because nothing is wrong with their credentials, and amplifies an outage into a stampede. The distinction is not guesswork -- the adapter already raises different types, and ClientError is a SIBLING of ServiceError rather than a parent: ClientError (4xx) this credential was refused -> prompt a re-link ServiceError (5xx) the auth gateway/service is at fault -> assume good, proceed anything else the CHECK broke, not the credential -> assume good, proceed 403 counts as a refusal on purpose: a valid session that can no longer use the stored account needs a re-link to pick up a current one. Deliberately non-destructive. Nothing is tombstoned or deleted on rejection, so a systemic 401 costs a bot-fallback turn plus a rate-limited nudge and recovers by itself when the upstream does. Revoking rows would not recover. Skipped entirely when AGENTEX_AUTH_URL is unset (local dev, authz off): nothing to verify against, and refusing every credential would make the feature untestable offline. Costs one in-cluster call per linked turn -- the same call the shared-bot path already makes every turn, so this is the existing cost profile rather than a new one. No caching, which would mean holding a "this credential was fine" verdict after it stopped being fine. Testing: 9 new unit tests covering all seven outcomes (accepted, 401, 403, two 5xx flavours, timeout, unexpected exception), plus that a rejection yields sgp_user_id=None -- the re-link trigger -- and that nothing calls revoke or delete. One trap worth recording: these tests patch resolve_environment_variable_dependency, not os.environ. That resolver reads a GlobalDependencies singleton built once per process, so monkeypatch.setenv never reaches it, and the first version of these tests passed for the wrong reason by silently taking the authz-disabled path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An unlinked mention produced two messages that contradicted each other. The gateway DM'd a connect link, and the turn ran anyway on the shared bot identity -- where the agent has no way to know it isn't the person who asked, so it reported on ITS OWN access as though it were theirs. Observed: confidently "No Linear access either -- verified two ways: no Linear tools in my toolset, and no Linear API key in my environment", alongside an overstated GitHub claim it later had to walk back. All true of the bot, none of it true of the user, and arriving while that user was being told to connect their account. The turn context now says so when sgp_user_id is None -- the same signal that triggers the link offer, so the agent is told it is unlinked exactly when the user is asked to link, and the two messages agree. The directive covers what the agent was getting wrong: - it is not running as the sender, and any personal integration it can reach belongs to the shared account - do not describe that access as if it were theirs, or conclude anything about what they have connected - answer from the thread and shared tools; if the request needs their data, say plainly that it needs their account connected and that a link has been sent It also removes the reason the agent invents its own authorization flow. Lacking a credential, the harness offers an OAuth URL with a localhost redirect (mcp.linear.app/authorize?...redirect_uri=http://localhost:52659/callback), which cannot work from Slack -- the redirect points at the agent's sandbox -- and is indistinguishable from a real instruction. The user then has two links, one real and one a dead end. Telling the agent a link has already been sent removes the reason to improvise one. No behavior change for linked turns: the flag is False and the context is byte-for- byte what it was. This is a mitigation, not a fix for the underlying thing. The gateway cannot stop the harness generating those URLs; it can only remove the situation that prompts them. The localhost-OAuth-link behaviour is worth fixing where it lives. Testing: 8 new unit tests. The context ones assert the disclaimer and the no-OAuth-links directive appear only when unlinked, that the channel id survives either way, that it composes with the self-posts directive (golden-agent needs both), and that the user's prompt still comes last so directives aren't read as part of the question. The wiring ones drive _dispatch far enough to capture the flag and assert it mirrors sgp_user_id, rather than inspecting source. 726 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sking again The flow was ask -> get a link -> click -> ASK AGAIN. The nonce has carried the triggering message as pending_turn since #410 for exactly this, and nothing used it, so the person's actual question was dropped at the moment we could finally answer it. A successful link now replays that turn as them. The replay lands in a NEW task for free, which is what makes this clean. The task key includes the SGP user id, so the same Slack thread keys differently once linked: slack:{ts} -> slack:{team}:{channel}:{ts}:{sgp_user} A different name is a different task, so the replay is turn 1 of a fresh session and picks up their credentials -- none of the toolset pinning that makes enabling an MCP mid-conversation a no-op. That pinning is what cost an hour of debugging earlier; here the key design sidesteps it without trying to. The pre-link exchange is not in the new task's history, but the agent can read the Slack thread with its own tools, which the context prefix already tells it how to do. Ordering is the load-bearing detail. The replay is scheduled LAST -- after the upsert, after the nonce is burned, after the cache is invalidated -- so it resolves the link it just created rather than a stale negative cache entry. Resolving stale would run the question as the shared bot and, without the guard below, offer another link. _run_turn takes offer_link=False for a replay. That turn exists BECAUSE the user just linked, so nudging again would be absurd; worse, offering would mint another nonce and DM another link, inviting the same loop on the next click. The guard makes the loop unrepresentable rather than merely unlikely. Scheduled in the background: this returns an HTML page to a waiting browser, and an agent turn takes far longer than a page load should. Best-effort by contract -- the link is already durable, and a replay that fails to schedule cannot undo it. It also sets the "thinking..." indicator, because the answer arrives minutes after the user clicked a web page and would otherwise appear from nowhere. The success page now says which happened: "I'm answering the message you sent in Slack now" when a replay was scheduled, "go back to Slack and ask again" when there was nothing to replay. Testing: 16 new unit tests. Route side: the replay is scheduled with the verified Slack identity from the nonce (not anything the browser supplied), nothing is scheduled without a pending turn or when the link was refused, and the ordering against upsert/consume/invalidate holds. Gateway side: the turn is reconstructed faithfully, the selector is re-derived exactly as normalize() does so a selector-driven turn resolves to the same target, the offer is suppressed, the status is set, and five shapes of incomplete pending turn plus a missing identity all no-op. 742 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
credential_expires_atis an upper bound, not a guarantee. A session JWT stopsbeing honoured the moment its owner signs out or it's revoked — while the stored expiry
still reads months away.
So today the gateway hands the agent a dead cookie, every user-scoped tool call 401s
where the agent can only report "I can't reach your Notion", and nothing ever
prompts a re-link, because locally the credential looks perfectly fine. Silent, and
indefinite.
A linked turn now verifies the stored session against agentex-auth before acting as
that user. A refusal falls back to the bot with
sgp_user_id=None— already the exactcondition that triggers a re-link offer, so there's no new wiring.
A rejection is not a failure
This is the part that needed care. A misconfigured or unavailable auth service refuses
everyone — and concluding "your credential is bad" would tell an entire workspace to
re-link, which fixes nothing (their credentials are fine) and amplifies an outage into a
stampede.
The distinction isn't guesswork. The adapter already raises different types, and
ClientErroris a sibling ofServiceError, not a parent:403counts as a refusal on purpose: a valid session that can no longer use the storedaccount needs a re-link to pick up a current one.
Deliberately non-destructive
Nothing is tombstoned or deleted on rejection. A systemic 401 therefore costs a
bot-fallback turn plus a rate-limited nudge, and recovers by itself when the upstream
does. Revoking rows would not recover.
Cost
One in-cluster call per linked turn — the same call the shared-bot path already makes on
every turn, so this is the existing cost profile rather than a new one.
No caching, deliberately: a cache here means holding a "this credential was fine" verdict
after it stopped being fine, which is the exact bug being fixed.
Skipped entirely when
AGENTEX_AUTH_URLis unset (local dev, authz off) — nothing toverify against, and refusing every credential would make the feature untestable offline.
Testing
33 new unit tests covering all seven outcomes — accepted, 401, 403, both 5xx flavours,
timeout, unexpected exception — plus two that pin the design rather than the mechanism:
sgp_user_id=None, which is what makes_run_turnoffer a re-linkrevokeordeleteon rejection742 unit tests pass.
ruffclean.resolve_environment_variable_dependency, notos.environ. That resolver reads aGlobalDependenciessingleton built once per process, somonkeypatch.setenvneverreaches it — the first version of these tests passed for the wrong reason, silently
taking the authz-disabled path and asserting nothing.
Also: the agent no longer speaks as if it were the user
An unlinked mention produced two messages that contradicted each other. The gateway
DM'd a connect link, and the turn ran anyway on the shared bot identity — where the agent
has no way to know it isn't the person who asked, so it reported on its own access as
though it were theirs:
All true of the bot, none of it true of the user, and arriving while that user was being
told to connect their account.
The turn context now says so when
sgp_user_id is None— the same signal that triggersthe link offer, so the agent is told it's unlinked exactly when the user is asked to
link, and the two messages agree.
It also removes the reason the agent invents its own authorization flow. Lacking a
credential, the harness offers an OAuth URL with a localhost redirect:
That cannot work from Slack — the redirect points at the agent's sandbox — and is
indistinguishable from a real instruction, so the user ends up with two links, one real
and one a dead end. Telling the agent a link has already been sent removes the reason to
improvise one.
A mitigation, not a cure. The gateway can't stop the harness generating those URLs;
it can only remove the situation that prompts them. That behaviour is worth fixing where
it lives.
No behaviour change for linked turns — the flag is
Falseand the context isbyte-for-byte what it was.
Also: linking now answers the question that prompted it
The flow was ask → get a link → click → ask again. The nonce has carried the
triggering message as
pending_turnsince #410 for exactly this purpose, and nothingused it — so the person's actual question got dropped at the moment we could finally
answer it.
The replay lands in a new task for free, which is what makes this clean. The task
key includes the SGP user id, so the same Slack thread keys differently once linked:
Different name, different task — so the replay is turn 1 of a fresh session and picks up
their credentials. None of the turn-1 toolset pinning that makes enabling an MCP
mid-conversation a no-op, and no need to start a new thread by hand. That pinning is what
cost an hour of debugging earlier in this work; the key design sidesteps it here without
having been designed to.
Ordering is the load-bearing detail. The replay is scheduled last — after the
upsert, after the nonce is burned, after the cache is invalidated — so it resolves the
link it just created rather than a stale negative cache entry. Resolving stale would run
the question as the shared bot and, without the guard below, offer another link.
offer_link=Falseon a replay. That turn exists because the user just linked, sonudging again would be absurd — and offering would mint another nonce and DM another
link, inviting the same loop on the next click. The guard makes the loop
unrepresentable rather than merely unlikely.
Scheduled in the background (this returns an HTML page to a waiting browser), sets the
"thinking…" indicator (the answer arrives minutes after clicking a web page and would
otherwise appear from nowhere), and best-effort by contract — the link is already
durable, so a replay that fails to schedule can't undo it.
What this doesn't cover
The check is on our side of the call. If the vault or an MCP rejects the credential
for a reason agentex-auth accepts, we won't notice — the agent still reports a tool
failure and the user gets no prompt. Closing that needs a signal back from the agent,
which doesn't exist. This covers the case that actually bites (sign-out), not every
possible rejection.
🤖 Generated with Claude Code
Greptile Summary
The PR validates stored Slack-linked sessions before user-scoped turns, clarifies shared-identity behavior to the agent, and replays the question that initiated a successful link.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as Slack user participant G as Slack gateway participant A as Auth service participant L as Link callback participant X as Agent U->>G: Send linked or unlinked message G->>A: Verify stored session alt Session accepted A-->>G: Principal accepted G->>X: Run as linked user else Session refused A-->>G: Authentication/authorization refusal G-->>U: Send account-link offer G->>X: Run with shared identity and unlinked context U->>L: Complete account link L->>L: Store link and invalidate cache L-->>U: Connected page L->>G: Background replay of pending message G->>X: Run replay using fresh link else Verification service fails A--xG: Service or unexpected failure G->>X: Proceed with stored linked credential endReviews (3): Last reviewed commit: "feat(agentex): answer the pending questi..." | Re-trigger Greptile
Context used: